Closed
Conversation
abdulanu0
requested review from
juliomenendez,
nikhilNava and
pontemonti
as code owners
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive unit test coverage for the notification models and agent notification functionality in the microsoft-agents-a365-notifications library. The tests cover model initialization, validation, property access, and routing behavior.
Key Changes:
- Added unit tests for NotificationTypes enum covering all enum operations and string comparisons
- Added unit tests for EmailReference and WpxComment models testing initialization, validation, and serialization
- Added unit tests for AgentNotificationActivity testing entity parsing and model conversion
- Added unit tests for AgentNotification class testing route matching and decorator registration
- Updated import path from
microsoft_agents.activitytomicrosoft_agents.activity.channel_idfor ChannelId
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/notifications/models/test_wpx_comment.py | Comprehensive tests for WpxComment model initialization, validation, and property access |
| tests/notifications/models/test_notification_types.py | Tests for NotificationTypes enum values, string operations, and enum behavior |
| tests/notifications/models/test_email_reference.py | Tests for EmailReference model covering initialization, validation, and HTML content handling |
| tests/notifications/models/test_agent_notification_activity.py | Tests for AgentNotificationActivity including entity parsing and model conversion |
| tests/notifications/models/test_agent_notification.py | Tests for AgentNotification class covering route matching, decorators, and subchannel handling |
| tests/notifications/models/init.py | Added copyright header and module docstring for test package |
| tests/notifications/init.py | Added copyright header and module docstring for test package |
| libraries/microsoft-agents-a365-notifications/microsoft_agents_a365/notifications/agent_notification.py | Updated ChannelId import path to be more specific |
joratz
previously approved these changes
Oct 28, 2025
...icrosoft-agents-a365-notifications/microsoft_agents_a365/notifications/agent_notification.py
Outdated
Show resolved
Hide resolved
tests/microsoft-agents-a365-notification/models/test_notification_types.py
Outdated
Show resolved
Hide resolved
tests/microsoft-agents-a365-notification/models/test_notification_types.py
Outdated
Show resolved
Hide resolved
tests/microsoft-agents-a365-notification/models/test_notification_types.py
Outdated
Show resolved
Hide resolved
tests/microsoft-agents-a365-notification/models/test_notification_types.py
Outdated
Show resolved
Hide resolved
tests/semantickernel_tests/test_semantickernel_service_logic.py
Outdated
Show resolved
Hide resolved
tests/microsoft-agents-a365-tooling-unittest/utils/test_utility.py
Outdated
Show resolved
Hide resolved
tests/microsoft-agents-a365-tooling-unittest/utils/test_utility.py
Outdated
Show resolved
Hide resolved
tests/microsoft-agents-a365-notification/models/test_wpx_comment.py
Outdated
Show resolved
Hide resolved
…y.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
tests/microsoft-agents-a365-notification/models/test_agent_notification.py
Outdated
Show resolved
Hide resolved
…ub.com/microsoft/Agent365-python into users/anabdul/Unittest_Notification_Py
tests/semantickernel_tests/test_semantickernel_service_logic.py
Outdated
Show resolved
Hide resolved
tests/semantickernel_tests/test_semantickernel_service_logic.py
Outdated
Show resolved
Hide resolved
tests/semantickernel_tests/test_semantickernel_service_logic.py
Outdated
Show resolved
Hide resolved
tests/microsoft-agents-a365-tooling-unittest/models/test_mcp_server_config.py
Outdated
Show resolved
Hide resolved
...icrosoft-agents-a365-tooling-unittest/services/test_mcp_tool_server_configuration_service.py
Outdated
Show resolved
Hide resolved
tests/microsoft-agents-a365-tooling-unittest/models/test_mcp_server_config.py
Outdated
Show resolved
Hide resolved
tests/microsoft-agents-a365-tooling-unittest/models/test_mcp_server_config.py
Outdated
Show resolved
Hide resolved
...icrosoft-agents-a365-tooling-unittest/services/test_mcp_tool_server_configuration_service.py
Outdated
Show resolved
Hide resolved
...icrosoft-agents-a365-tooling-unittest/services/test_mcp_tool_server_configuration_service.py
Outdated
Show resolved
Hide resolved
...-agents-a365-tooling-extensions-semantickernel-unittest/test_semantickernel_service_logic.py
Show resolved
Hide resolved
tests/microsoft-agents-a365-tooling-extensions-openai-unittest/test_openai_service_logic.py
Show resolved
Hide resolved
...-agents-a365-tooling-extensions-azureaifoundry-unittest/test_azureaifoundry_service_logic.py
Show resolved
Hide resolved
...icrosoft-agents-a365-tooling-unittest/services/test_mcp_tool_server_configuration_service.py
Show resolved
Hide resolved
tests/microsoft-agents-a365-tooling-unittest/models/test_mcp_server_config.py
Outdated
Show resolved
Hide resolved
tests/microsoft-agents-a365-notification/models/test_notification_types.py
Show resolved
Hide resolved
tests/microsoft-agents-a365-tooling-unittest/models/test_mcp_server_config.py
Outdated
Show resolved
Hide resolved
- Standardized all test files to use 'Copyright (c) Microsoft Corporation.' - Added 'Licensed under the MIT License.' to comply with Microsoft coding standards - Updated 9 test files across notification and tooling unittest modules - Ensures consistency with Microsoft open source project requirements
tests/microsoft-agents-a365-notification/models/test_agent_notification.py
Outdated
Show resolved
Hide resolved
...-agents-a365-tooling-extensions-azureaifoundry-unittest/test_azureaifoundry_service_logic.py
Show resolved
Hide resolved
...-agents-a365-tooling-extensions-azureaifoundry-unittest/test_azureaifoundry_service_logic.py
Outdated
Show resolved
Hide resolved
...-agents-a365-tooling-extensions-azureaifoundry-unittest/test_azureaifoundry_service_logic.py
Outdated
Show resolved
Hide resolved
...-agents-a365-tooling-extensions-azureaifoundry-unittest/test_azureaifoundry_service_logic.py
Outdated
Show resolved
Hide resolved
tests/microsoft-agents-a365-tooling-unittest/utils/test_utility.py
Outdated
Show resolved
Hide resolved
Comment on lines
79
to
81
| def get_ppapi_token_scope(): | ||
| """ | ||
| Gets the MCP platform authentication scope based on the current environment. |
There was a problem hiding this comment.
The function name get_ppapi_token_scope doesn't match the docstring which refers to 'MCP platform authentication scope'. The docstring should be updated to reflect that this returns the Power Platform API token scope.
Comment on lines
+55
to
+57
| assert email_string == "Type: NotificationTypes.EMAIL_NOTIFICATION" | ||
| assert wpx_string == "Type: NotificationTypes.WPX_COMMENT" | ||
| assert lifecycle_string == "Type: NotificationTypes.AGENT_LIFECYCLE" |
There was a problem hiding this comment.
The test expects enum string formatting to include the enum class name (e.g., 'NotificationTypes.EMAIL_NOTIFICATION'), but Python's f-string formatting with enum values typically just returns the value itself (e.g., 'emailNotification'). Unless the enum has a custom __str__ method, this test will likely fail.
rahuldevikar761
requested changes
Nov 13, 2025
Collaborator
rahuldevikar761
left a comment
rahuldevikar761
left a comment
There was a problem hiding this comment.
Suggested feedback on the chat. Please take a look
Comment on lines
+1
to
+5
| # Copyright (c) Microsoft. All rights reserved. | ||
|
|
||
| """ | ||
| Unit tests for McpToolRegistrationService business logic. | ||
| This tests the service implementation flow with mock dependencies. |
There was a problem hiding this comment.
File is missing the standard Microsoft copyright header. Please update the header to:
Copyright (c) Microsoft Corporation.
Licensed under the MIT License.
Add a blank line after the header.
Comment on lines
+1
to
+5
| # Copyright (c) Microsoft. All rights reserved. | ||
|
|
||
| """ | ||
| Unit tests for Microsoft Agents A365 Notifications module. | ||
| """ |
There was a problem hiding this comment.
File is missing the standard Microsoft copyright header. Please update the header to:
Copyright (c) Microsoft Corporation.
Licensed under the MIT License.
Add a blank line after the header.
Comment on lines
+1
to
+5
| # Copyright (c) Microsoft. All rights reserved. | ||
|
|
||
| """ | ||
| Unit tests for SemanticKernel MCP Tool Registration Service core logic. | ||
| """ |
There was a problem hiding this comment.
File is missing the standard Microsoft copyright header. Please update the header to:
Copyright (c) Microsoft Corporation.
Licensed under the MIT License.
Add a blank line after the header.
Comment on lines
+1
to
+5
| # Copyright (c) Microsoft. All rights reserved. | ||
|
|
||
| """ | ||
| Unit tests for AgentNotificationActivity class - Clean Version | ||
| """ |
There was a problem hiding this comment.
File is missing the standard Microsoft copyright header. Please update the header to:
Copyright (c) Microsoft Corporation.
Licensed under the MIT License.
Add a blank line after the header.
Comment on lines
+1
to
+5
| # Copyright (c) Microsoft. All rights reserved. | ||
|
|
||
| """ | ||
| Unit tests for Microsoft Agents A365 Notifications models. | ||
| """ |
There was a problem hiding this comment.
File is missing the standard Microsoft copyright header. Please update the header to:
Copyright (c) Microsoft Corporation.
Licensed under the MIT License.
Add a blank line after the header.
Comment on lines
+1
to
+5
| # Copyright (c) Microsoft. All rights reserved. | ||
|
|
||
| """ | ||
| Unit tests for OpenAI MCP Tool Registration Service core logic. | ||
| """ |
There was a problem hiding this comment.
File is missing the standard Microsoft copyright header. Please update the header to:
Copyright (c) Microsoft Corporation.
Licensed under the MIT License.
Add a blank line after the header.
Comment on lines
+1
to
+5
| # Copyright (c) Microsoft. All rights reserved. | ||
|
|
||
| """ | ||
| Unit tests for AgentNotification class | ||
| """ |
There was a problem hiding this comment.
File is missing the standard Microsoft copyright header. Please update the header to:
Copyright (c) Microsoft Corporation.
Licensed under the MIT License.
Add a blank line after the header.
Comment on lines
+1
to
+5
| # Copyright (c) Microsoft. All rights reserved. | ||
|
|
||
| """ | ||
| Init file for services tests. | ||
| """ |
There was a problem hiding this comment.
File is missing the standard Microsoft copyright header. Please update the header to:
Copyright (c) Microsoft Corporation.
Licensed under the MIT License.
Add a blank line after the header.
Comment on lines
+1
to
+5
| # Copyright (c) Microsoft. All rights reserved. | ||
|
|
||
| """ | ||
| Init file for models tests. | ||
| """ |
There was a problem hiding this comment.
File is missing the standard Microsoft copyright header. Please update the header to:
Copyright (c) Microsoft Corporation.
Licensed under the MIT License.
Add a blank line after the header.
Comment on lines
+1
to
+5
| # Copyright (c) Microsoft. All rights reserved. | ||
|
|
||
| """ | ||
| Unit tests for microsoft-agents-a365-tooling library. | ||
| """ |
There was a problem hiding this comment.
File is missing the standard Microsoft copyright header. Please update the header to:
Copyright (c) Microsoft Corporation.
Licensed under the MIT License.
Add a blank line after the header.
| @@ -0,0 +1,5 @@ | |||
| # Copyright (c) Microsoft. All rights reserved. | |||
There was a problem hiding this comment.
Please update the header to:
Copyright (c) Microsoft Corporation.
Licensed under the MIT License.
Keep a blank line after the header.
| @@ -0,0 +1,255 @@ | |||
| # Copyright (c) Microsoft. All rights reserved. | |||
There was a problem hiding this comment.
Please update the header to:
Copyright (c) Microsoft Corporation.
Licensed under the MIT License.
Followed by a blank line.
Copilot AI
added a commit
that referenced
this pull request
Mar 16, 2026
… asserts, and more - Fix #1: Downgrade sensitive data logging from INFO to DEBUG in agent365_exporter.py - Fix #2: Fix unpaired context.attach() in opentelemetry_scope.py add_baggage() by storing and detaching baggage tokens on scope end - Fix #3: Add bounded OrderedDict caps to unbounded dicts in OpenAI trace_processor.py - Fix #4: Replace 30 assert statements with proper TypeError raises in LangChain utils.py - Fix #5: Log security warning when HTTP domain override is detected - Fix #6: Warn when bearer token sent over non-HTTPS connection - Fix #10: Respect Retry-After header and use exponential backoff in retries - Fix #13: Rename reset() to _reset() in ObservabilityHostingManager - Fix #15: Replace print() with logger.warning() in LangChain tracer_instrumentor.py Co-authored-by: nikhilNava <211831449+nikhilNava@users.noreply.github.com>
nikhilNava
added a commit
that referenced
this pull request
Mar 17, 2026
* Initial plan * Replace assert statements with explicit TypeError raises in langchain utils Replace all 30 assert statements in utils.py with equivalent if-not-raise TypeError checks. This ensures type validation is not silently stripped when Python runs with -O (optimized mode). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * security: fix sensitive data logging, context leak, unbounded memory, asserts, and more - Fix #1: Downgrade sensitive data logging from INFO to DEBUG in agent365_exporter.py - Fix #2: Fix unpaired context.attach() in opentelemetry_scope.py add_baggage() by storing and detaching baggage tokens on scope end - Fix #3: Add bounded OrderedDict caps to unbounded dicts in OpenAI trace_processor.py - Fix #4: Replace 30 assert statements with proper TypeError raises in LangChain utils.py - Fix #5: Log security warning when HTTP domain override is detected - Fix #6: Warn when bearer token sent over non-HTTPS connection - Fix #10: Respect Retry-After header and use exponential backoff in retries - Fix #13: Rename reset() to _reset() in ObservabilityHostingManager - Fix #15: Replace print() with logger.warning() in LangChain tracer_instrumentor.py Co-authored-by: nikhilNava <211831449+nikhilNava@users.noreply.github.com> * Restore agent/tenant IDs and response text in exporter log messages Agent IDs and tenant IDs are not sensitive data and are useful for debugging. Restore them in debug/error log messages. Also restore truncated response text in HTTP error logs to help developers debug failures. Log levels remain at DEBUG (from the prior security fix). Co-authored-by: nikhilNava <211831449+nikhilNava@users.noreply.github.com> * Remove add_baggage() from OpenTelemetryScope The method had an unpaired context.attach() that leaked context tokens. Users should use BaggageBuilder.build() context manager instead, which properly restores the previous context on exit. Co-authored-by: nikhilNava <211831449+nikhilNava@users.noreply.github.com> * Security hardening for observability packages Co-authored-by: nikhilNava <211831449+nikhilNava@users.noreply.github.com> * Fix formatting in test_agent365_exporter.py and replace remaining raise TypeError with isinstance guards in langchain utils.py Co-authored-by: nikhilNava <211831449+nikhilNava@users.noreply.github.com> * Move _parse_retry_after to exporters/utils.py as standalone parse_retry_after function Co-authored-by: nikhilNava <211831449+nikhilNava@users.noreply.github.com> * Replace type(e).__name__ with str(e) in exporter error logging per PR review Co-authored-by: nikhilNava <211831449+nikhilNava@users.noreply.github.com> * feat: add bounded collections for LangChain tracer and OutputScope - Convert LangChain _spans_by_run from unbounded DictWithLock to bounded OrderedDict with _MAX_TRACKED_RUNS=10000 cap - Add _cap_ordered_dict helper for FIFO eviction (matching OpenAI pattern) - Add thread-safe lock usage for _spans_by_run in error handlers - Add _MAX_OUTPUT_MESSAGES=5000 cap for OutputScope._output_messages - Add unit tests for both bounded collections Co-authored-by: nikhilNava <211831449+nikhilNava@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: nikhilNava <211831449+nikhilNava@users.noreply.github.com> Co-authored-by: Nikhil Navakiran <nikhil.navakiran@gmail.com>
biswapm
pushed a commit
to biswapm/Agent365-python
that referenced
this pull request
Mar 20, 2026
* Initial plan * Replace assert statements with explicit TypeError raises in langchain utils Replace all 30 assert statements in utils.py with equivalent if-not-raise TypeError checks. This ensures type validation is not silently stripped when Python runs with -O (optimized mode). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * security: fix sensitive data logging, context leak, unbounded memory, asserts, and more - Fix microsoft#1: Downgrade sensitive data logging from INFO to DEBUG in agent365_exporter.py - Fix microsoft#2: Fix unpaired context.attach() in opentelemetry_scope.py add_baggage() by storing and detaching baggage tokens on scope end - Fix microsoft#3: Add bounded OrderedDict caps to unbounded dicts in OpenAI trace_processor.py - Fix microsoft#4: Replace 30 assert statements with proper TypeError raises in LangChain utils.py - Fix microsoft#5: Log security warning when HTTP domain override is detected - Fix microsoft#6: Warn when bearer token sent over non-HTTPS connection - Fix microsoft#10: Respect Retry-After header and use exponential backoff in retries - Fix microsoft#13: Rename reset() to _reset() in ObservabilityHostingManager - Fix microsoft#15: Replace print() with logger.warning() in LangChain tracer_instrumentor.py Co-authored-by: nikhilNava <211831449+nikhilNava@users.noreply.github.com> * Restore agent/tenant IDs and response text in exporter log messages Agent IDs and tenant IDs are not sensitive data and are useful for debugging. Restore them in debug/error log messages. Also restore truncated response text in HTTP error logs to help developers debug failures. Log levels remain at DEBUG (from the prior security fix). Co-authored-by: nikhilNava <211831449+nikhilNava@users.noreply.github.com> * Remove add_baggage() from OpenTelemetryScope The method had an unpaired context.attach() that leaked context tokens. Users should use BaggageBuilder.build() context manager instead, which properly restores the previous context on exit. Co-authored-by: nikhilNava <211831449+nikhilNava@users.noreply.github.com> * Security hardening for observability packages Co-authored-by: nikhilNava <211831449+nikhilNava@users.noreply.github.com> * Fix formatting in test_agent365_exporter.py and replace remaining raise TypeError with isinstance guards in langchain utils.py Co-authored-by: nikhilNava <211831449+nikhilNava@users.noreply.github.com> * Move _parse_retry_after to exporters/utils.py as standalone parse_retry_after function Co-authored-by: nikhilNava <211831449+nikhilNava@users.noreply.github.com> * Replace type(e).__name__ with str(e) in exporter error logging per PR review Co-authored-by: nikhilNava <211831449+nikhilNava@users.noreply.github.com> * feat: add bounded collections for LangChain tracer and OutputScope - Convert LangChain _spans_by_run from unbounded DictWithLock to bounded OrderedDict with _MAX_TRACKED_RUNS=10000 cap - Add _cap_ordered_dict helper for FIFO eviction (matching OpenAI pattern) - Add thread-safe lock usage for _spans_by_run in error handlers - Add _MAX_OUTPUT_MESSAGES=5000 cap for OutputScope._output_messages - Add unit tests for both bounded collections Co-authored-by: nikhilNava <211831449+nikhilNava@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: nikhilNava <211831449+nikhilNava@users.noreply.github.com> Co-authored-by: Nikhil Navakiran <nikhil.navakiran@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adding unit tests to tooling, tooling extensions as well as notifications